[Image] Add onLoadStart, onLoadProgress, onLoadError events to Image component #1318

ptmt · 2015-05-17T11:47:17Z

This PR adds 4 native events to NetworkImage.

Using these events I could wrap Image component into something like:

class NetworkImage extends React.Component {
  getInitialState() {
    return {
      downloading: false,
      progress: 0
    }
  }

  render() {
    var loader = this.state.downloading ?
      <View style={this.props.loaderStyles}>
        <ActivityIndicatorIOS animating={true} size={'large'} />
        <Text style={{color: '#bbb'}}>{this.state.progress}%</Text>
      </View>
      :
      null;

    return <Image source={this.props.source}
      onLoadStart={() => this.setState({downloading: true}) }
      onLoaded={() => this.setState({downloading: false}) }
      onLoadProgress={(e)=> this.setState({progress: Math.round(100 * e.nativeEvent.written / e.nativeEvent.total)});
      onLoadError={(e)=> {
        alert('the image cannot be downloaded because: ', JSON.stringify(e));
        this.setState({downloading: false});
      }}>
      {loader}
    </Image>
  }
}

Useful on slow connections and server errors.

There are dozen lines of Objective C, which I don't have experience with. There are neither specific tests nor documentation yet. And I do realize that you're already working right now on better <Image/> (pipeline, new asset management, etc.). So this is basically a proof concept of events for images, and if this idea is not completely wrong I could improve it or help somehow.

a2 · 2015-05-18T15:57:55Z

Libraries/Image/RCTDownloadTaskWrapper.h

+
+@interface RCTDownloadTaskWrapper : NSObject <NSURLSessionDownloadDelegate>
+
+@property(nonatomic,assign) id<NSURLSessionDownloadDelegate> delegate;


Nit: One space after @property.

Delegates should have "weak" retain semantics.

a2 · 2015-05-18T16:00:48Z

Sorry for all the nitpicks.

ptmt · 2015-05-19T12:43:04Z

Thank you so much! Could you help me about tests and other possible performances issues? As you mentioned before NSURLSession which creating each time. For now I use it in the app with a lot of images loading - everything is fine, but I understand these changes could affect somebody. Or should I move it in external library?

a2 · 2015-05-19T12:49:05Z

Libraries/Image/RCTImageDownloader.m

 {
  NSString *cacheKey = RCTCacheKeyForURL(url);
  __weak RCTImageDownloader *weakSelf = self;
-  return [self _downloadDataForURL:url block:^(BOOL cached, NSData *data, NSError *error) {
+  return [self _downloadDataForURL:url progressBlock:(RCTDataProgressBlock)progressBlock block:^(BOOL cached, NSData *data, NSError *error) {


You don't need to cast the blocks.

brentvatne · 2015-05-27T02:34:48Z

Thanks for all of the work here people, looking forward to seeing this in 😄

ptmt · 2015-05-29T13:32:40Z

Merged from upstream (new lines about resizing etc), I've got unnecessary commits, it looks unclean now. I use this branch in production, though, don't know what to do next exactly. Tests? Rebase?

brentvatne · 2015-05-29T17:25:28Z

@a2 - any chance you have a bit of time to ❤️ this PR?

ide · 2015-05-29T18:18:18Z

As for the API you may want to study https://html.spec.whatwg.org/multipage/embedded-content.html#mediaevents to see if any of the concepts are worth sharing (cc @vjeux)

ptmt · 2015-05-30T12:33:16Z

@ide thanks, good point. I consider to adding the abort event. And about events' naming conventions, not sure how to do it right. I've used https://facebook.github.io/react-native/docs/textinput.html#onchange pattern on{eventName}. It's also similar to the current state of https://github.com/brentvatne/react-native-video, but it makes sense to rename methods. So we will have:
loadstart, progress, abort, error, loadeddata.

brentvatne · 2015-05-31T20:22:23Z

@vjeux - can you shed any light on how you'd like to do naming for these types of events, as per @UnknownException comment above?

browniefed · 2015-06-01T03:41:21Z

This would be super useful, especially if the loaded data got passed back on success in base64 encoded string or some workable format.

a2 · 2015-06-01T10:53:57Z

Waiting on event naming convention comments.

vjeux · 2015-06-01T20:47:52Z

I like the naming from the spec:

onLoadStart
onLoadProgress
onLoadAbort
onLoadError
onLoaded

This would be super useful, especially if the loaded data got passed back on success in base64 encoded string or some workable format.

No. We never want to read image data in js. This means sending a huge chunk of memory (several megabytes) to js. There are some places where js is not a good language and doing image processing is one of them. You should be manipulating the image using C or the GPU. Not js with a base64 encoded file.

vjeux · 2015-06-01T20:50:58Z

Libraries/Image/RCTNetworkImageView.m

-      _downloadToken = [_imageDownloader downloadImageForURL:imageURL size:self.bounds.size scale:RCTScreenScale() resizeMode:self.contentMode backgroundColor:self.backgroundColor block:^(UIImage *image, NSError *error) {
+      _downloadToken = [_imageDownloader downloadImageForURL:imageURL size:self.bounds.size scale:RCTScreenScale()
+                                                  resizeMode:self.contentMode backgroundColor:self.backgroundColor
+                                                  progressBlock:progressHandler block:^(UIImage *image, NSError *error) {


Can you not update progress if there is no onProgress callback setup in js? This is wasteful to send events that are not going to be handled. (I'm not familiar with iOS so i'm not sure if you already do that, sorry if it's the case)

Agreed. There is a slightly tricky edge case to handle: on the Image's first render pass we don't provide an onProgress handler, and then provide one on the second pass. So the view/viewmanager needs to enable/disable the progress events when the onProgress handler prop is set/removed.

Thanks for comments, I renamed events and tried to not fire the progress event if it's not defined.

Tested this behavior with something like this:

class TestNetworkImage extends Component { componentDidMount() { setTimeout(() => { this.setState({ onProgress: this.onProgress.bind(this) }) }, 1000); } render() { return <Image ref="image" source={{uri: this.props.source}} onLoadProgress={this.state.onProgress}> {this.props.loader} {this.props.children} </Image> } }

NSLog shows that at first second ImageView doesn't send an event to js in that case, then it does.

ptmt · 2015-07-07T07:59:12Z

The app which uses these changes has passed Apple review and available on AppStore. It's not a good thing to maintain the fork, so I need to figure out how to split it into the stand-alone plugin.

brentvatne · 2015-07-07T19:50:20Z

@UnknownException - I would love to see this merged upstream, @a2 - do you have any time to review again?

Also, @UnknownException - can you please rebase?

a2 · 2015-07-07T19:51:39Z

@facebook-github-bot import

facebook-github-bot · 2015-07-07T20:14:42Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/470962509718878/int_phab to review.

brentvatne · 2015-07-08T00:05:58Z

❤️ @a2

ptmt · 2015-07-08T15:08:42Z

Synced with the master and rebased. I had to adopt the new caching mechanism (NSURLCache) to make this whole thing work with NSURLSessionDownloadTask instead of NSURLSessionDataTask. Please, review, if you can.
Should I add examples to UIExplore?

brentvatne · 2015-07-10T19:17:44Z

@UnknownException - if you can add an example to UIExplorer and submit in a separate PR that would be lovely!

There are some code snippets that use `var` for declaring `styles` whereas some other snippets use `const`. Modified the page to all use `const` for consistency.

…cebook#439)" (facebook#1318) This change reverts microsoft#459 - but still tries to address the original issues: - microsoft#422 - microsoft#322 This also addresses an issue when programmatically resizing windows where the RCTRootContentView may end up at the wrong size because an in-flight layout gets resolved after the resize on the main thread. We now keep sync dispatch on the shadow queue for live resizing windows (to prevent tearing) but also dispatch async (as done on iOS) so the latest new size is sure to win. The block has a check to bail if the size doesn't change, so this isn't a perf drain running the block twice. Co-authored-by: Scott Kyle <skyle@fb.com>

reactjs-bot added the GH Review: review-needed label May 17, 2015

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 17, 2015

ptmt mentioned this pull request May 17, 2015

How to get the progress of the downloaded Image? #1172

Closed

a2 reviewed May 18, 2015
View reviewed changes

a2 self-assigned this May 18, 2015

a2 reviewed May 19, 2015
View reviewed changes

ptmt mentioned this pull request May 21, 2015

Images do not load on the ListView in iPhone 5 but works on iPhone 6 and up #1345

Closed

brentvatne changed the title ~~Add onDownloadProgress, onError events to Image component~~ [Image] Add onDownloadProgress, onError events to Image component May 30, 2015

vjeux added GH Review: needs-revision and removed GH Review: review-needed labels Jun 1, 2015

vjeux reviewed Jun 1, 2015
View reviewed changes

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Jun 2, 2015

ptmt changed the title ~~[Image] Add onDownloadProgress, onError events to Image component~~ [Image] Add onLoadStart, onLoadProgress, onLoadError events to Image component Jul 8, 2015

ptmt force-pushed the feature/image_events branch from 1ea70c1 to 3aed7c2 Compare July 8, 2015 14:50

[Image] add onLoadStart, onLoadProgress, onLoadError, onLoadEnd

7c856f5

ptmt force-pushed the feature/image_events branch from 3aed7c2 to 7c856f5 Compare July 8, 2015 15:04

nicklockwood closed this in 8e70c7f Jul 10, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Image] Add onLoadStart, onLoadProgress, onLoadError events to Image component #1318

[Image] Add onLoadStart, onLoadProgress, onLoadError events to Image component #1318

ptmt commented May 17, 2015

a2 May 18, 2015

a2 May 18, 2015

a2 commented May 18, 2015

ptmt commented May 19, 2015

a2 May 19, 2015

ptmt May 19, 2015

brentvatne commented May 27, 2015

ptmt commented May 29, 2015

brentvatne commented May 29, 2015

ide commented May 29, 2015

ptmt commented May 30, 2015

brentvatne commented May 31, 2015

browniefed commented Jun 1, 2015

a2 commented Jun 1, 2015

vjeux commented Jun 1, 2015

vjeux Jun 1, 2015

ide Jun 1, 2015

ptmt Jun 2, 2015

ptmt commented Jul 7, 2015

brentvatne commented Jul 7, 2015

a2 commented Jul 7, 2015

facebook-github-bot commented Jul 7, 2015

brentvatne commented Jul 8, 2015

ptmt commented Jul 8, 2015

brentvatne commented Jul 10, 2015


		@interface RCTDownloadTaskWrapper : NSObject <NSURLSessionDownloadDelegate>

		@property(nonatomic,assign) id<NSURLSessionDownloadDelegate> delegate;

[Image] Add onLoadStart, onLoadProgress, onLoadError events to Image component #1318

[Image] Add onLoadStart, onLoadProgress, onLoadError events to Image component #1318

Conversation

ptmt commented May 17, 2015

a2 May 18, 2015

Choose a reason for hiding this comment

a2 May 18, 2015

Choose a reason for hiding this comment

a2 commented May 18, 2015

ptmt commented May 19, 2015

a2 May 19, 2015

Choose a reason for hiding this comment

ptmt May 19, 2015

Choose a reason for hiding this comment

brentvatne commented May 27, 2015

ptmt commented May 29, 2015

brentvatne commented May 29, 2015

ide commented May 29, 2015

ptmt commented May 30, 2015

brentvatne commented May 31, 2015

browniefed commented Jun 1, 2015

a2 commented Jun 1, 2015

vjeux commented Jun 1, 2015

vjeux Jun 1, 2015

Choose a reason for hiding this comment

ide Jun 1, 2015

Choose a reason for hiding this comment

ptmt Jun 2, 2015

Choose a reason for hiding this comment

ptmt commented Jul 7, 2015

brentvatne commented Jul 7, 2015

a2 commented Jul 7, 2015

facebook-github-bot commented Jul 7, 2015

brentvatne commented Jul 8, 2015

ptmt commented Jul 8, 2015

brentvatne commented Jul 10, 2015